chore: report telemetry consent #540
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## development #540 +/- ##
==============================================
Coverage ? 52.65%
==============================================
Files ? 228
Lines ? 5320
Branches ? 835
==============================================
Hits ? 2801
Misses ? 2281
Partials ? 238 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
We're specifically anonymizing the consent response/change and that's why we're using a hardcoded user ID, right? That's just to gauge how many people are responding Y/N?
If the data reported is anonymous anyway, wouldn't we want to already know who changed the consent? We can sort of already understand who agreed (by inspecting the machine ID); this just prevents us from seeing when some machine ID stopped consenting (which is fine but wanted to bring it up).
Otherwise, looks good to me!
My understanding is that we want to anonymize this particular response because if the user answers |
antico5
force-pushed
the
telemetry_consent_reporting
branch
from
b183cc0
to
4b5d843
Compare
There was a problem hiding this comment.
Left one question.
But I think @kanej would be able to review this better.
Thanks!
| @@ -44,4 +38,6 @@ export async function showAnalyticsAllowPopup({ | |||
| await config.update("telemetry", isAccepted, ConfigurationTarget.Global); | |||
|
|
|||
| await context.globalState.update(PREVIOUSLY_SHOWN_TELEMETRY_LABEL, true); | |||
|
|
|||
| client?.sendNotification("custom/telemetryConsent", isAccepted); | |||
There was a problem hiding this comment.
question: I wonder if this change will record the consent status for people who already gave (or rejected) telemetry before? would we collect this data for older users?
There was a problem hiding this comment.
My reading of the code is that people who have previously made a decision on telemetry will not sendNotification to Google Analytics. It is only new people going through the complete process who will send this notification.
kanej
force-pushed
the
telemetry_consent_reporting
branch
from
4b5d843
to
efb3280
Compare
| @@ -13,7 +13,7 @@ const SENTRY_CLOSE_TIMEOUT = 2000; | |||
| export class SentryServerTelemetry implements Telemetry { | |||
| private dsn: string; | |||
| private serverState: ServerState | null; | |||
| private analytics: Analytics; | |||
There was a problem hiding this comment.
I get the move here, but I suspect we should either:
- Add analytics as a property of the serverState and call it directly (SentryTelemetry used to be the only place analytics was used, that is no longer the case). Though this likely means reworking
initand heartbeat. - Do this analytics changes on the client
This PR introduces two events sent to Google Analytics, without any client information, for when the user first responds to the "enable telemetry" popup, and any subsequent change to the setting via UI.